fix: Add objectParser for ObjectTypeAnnotation (e.g. PARSE_SERVER_AUTH_PROVIDERS) #9912
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdded ObjectTypeAnnotation handling to mapperFor (returning Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant Mapper as buildConfigDefinitions.mapperFor
participant Parsers as parsers
participant Options as ParseServerOptions.auth
note over Env,Mapper `#E8F6FF`: Build-time mapping of type annotations
Env->>Mapper: Provide type annotation node (e.g., ObjectTypeAnnotation)
alt ObjectTypeAnnotation
Mapper-->>Parsers: map -> parsers.objectParser
else AnyTypeAnnotation
Mapper-->>Parsers: map -> parsers.anyParser
else ArrayTypeAnnotation
Mapper-->>Parsers: map -> parsers.arrayParser
end
Parsers-->>Options: set action = parsers.objectParser
note over Parsers,Options `#E8F6FF`: Runtime env string → object via objectParser
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
resources/buildConfigDefinitions.js (1)
176-242: Consider adding ObjectTypeAnnotation handling for future-proofing.The
parseDefaultValuefunction handles various type annotations but doesn't include a case forObjectTypeAnnotation. While this isn't currently an issue (since theauthoption likely doesn't have a default value), adding this handling would make the function more complete and prevent potential issues if default values are added to object-type fields in the future.Consider adding after line 214:
} else if (t.isBooleanTypeAnnotation(elt)) { literalValue = t.booleanLiteral(parsers.booleanParser(value)); + } else if (t.isObjectTypeAnnotation(elt)) { + const object = parsers.objectParser(value); + const props = Object.keys(object).map(key => { + const val = object[key]; + if (typeof val === 'string') { + return t.objectProperty(t.identifier(key), t.stringLiteral(val)); + } else if (typeof val === 'number') { + return t.objectProperty(t.identifier(key), t.numericLiteral(val)); + } else if (typeof val === 'boolean') { + return t.objectProperty(t.identifier(key), t.booleanLiteral(val)); + } else { + return t.objectProperty(t.identifier(key), t.identifier('undefined')); + } + }); + literalValue = t.objectExpression(props); } else if (t.isGenericTypeAnnotation(elt)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/buildConfigDefinitions.js(1 hunks)src/Options/Definitions.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Options/Definitions.js
🧬 Code graph analysis (1)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Node 20
- GitHub Check: Node 22
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Docker Build
🔇 Additional comments (2)
src/Options/Definitions.js (1)
110-115: LGTM! Generated code correctly adds objectParser action.The addition of
action: parsers.objectParserto theauthoption is consistent with other object-type configuration options throughout this file (e.g.,accountLockout,customPages,fileUpload). This ensures that thePARSE_SERVER_AUTH_PROVIDERSenvironment variable is properly parsed as a JSON object rather than treated as a raw string, which directly addresses the PR objective.Note: This is generated code. The actual source changes are in
src/Options/index.jsand the generator scriptresources/buildConfigDefinitions.js.resources/buildConfigDefinitions.js (1)
156-157: LGTM! ObjectTypeAnnotation handling correctly added.The addition of
ObjectTypeAnnotationhandling inmapperForis correct and consistent with the existingAnyTypeAnnotationhandling. This ensures Flow object types (e.g.,?{ [string]: AuthAdapter }) are properly mapped toobjectParser.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9912 +/- ##
==========================================
- Coverage 93.07% 92.94% -0.14%
==========================================
Files 187 188 +1
Lines 15224 15414 +190
Branches 177 177
==========================================
+ Hits 14170 14326 +156
- Misses 1042 1076 +34
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…into fix/auth-env-bug
…rse-server into fix/auth-env-bug
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/buildConfigDefinitions.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
🧬 Code graph analysis (1)
spec/buildConfigDefinitions.spec.js (1)
resources/buildConfigDefinitions.js (1)
wrap(148-148)
🔇 Additional comments (1)
spec/buildConfigDefinitions.spec.js (1)
40-185: Good test coverage for the new ObjectTypeAnnotation mapping.The test suite provides comprehensive coverage of the
mapperForfunction, including the newly addedObjectTypeAnnotationmapping that this PR introduces. All major type annotation cases are covered.However, please address the issues raised in previous comments regarding:
- Importing the actual implementation instead of recreating it locally
- Correcting the Babel AST structure access patterns
- Verifying the source of the
nameproperty
spec/buildConfigDefinitions.spec.js
Outdated
| } else if (t.isObjectTypeAnnotation(elt)) { | ||
| return wrap(t.identifier('objectParser')); | ||
| } else if (t.isGenericTypeAnnotation(elt)) { | ||
| const type = elt.typeAnnotation.id.name; |
There was a problem hiding this comment.
Fix incorrect Babel AST path for GenericTypeAnnotation.
According to Babel AST specifications, GenericTypeAnnotation nodes have an id property directly, not nested under typeAnnotation. The correct access should be elt.id.name.
Apply this diff:
- const type = elt.typeAnnotation.id.name;
+ const type = elt.id.name;Additionally, update all test mock structures to match the correct Babel AST format. For example, at lines 104-108:
const mockElement = {
type: 'GenericTypeAnnotation',
- typeAnnotation: {
- id: {
- name: 'Adapter',
- },
+ id: {
+ name: 'Adapter',
},
};Apply similar corrections to all GenericTypeAnnotation mocks in tests (lines 104-108, 121-125, 139-143, 156-160, 173-177).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const type = elt.typeAnnotation.id.name; | |
| const type = elt.id.name; |
🤖 Prompt for AI Agents
In spec/buildConfigDefinitions.spec.js around line 21, change usage of
elt.typeAnnotation.id.name to elt.id.name because GenericTypeAnnotation nodes
expose id directly; update the code accordingly and then update all test mock
AST structures to match Babel's GenericTypeAnnotation shape (replace nested
typeAnnotation.id with id) for the mocks at lines 104-108, 121-125, 139-143,
156-160, and 173-177 so the tests construct GenericTypeAnnotation nodes with an
id property that has a name.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/buildConfigDefinitions.js(2 hunks)spec/buildConfigDefinitions.spec.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/buildConfigDefinitions.spec.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
resources/buildConfigDefinitions.js
🔇 Additional comments (1)
resources/buildConfigDefinitions.js (1)
158-159: LGTM! ObjectTypeAnnotation handling correctly implemented.The new handling for
ObjectTypeAnnotationcorrectly maps Flow object types toobjectParser, which fixes the issue where environment variables likePARSE_SERVER_AUTH_PROVIDERSwere treated as raw strings. The implementation is consistent with the existingAnyTypeAnnotationhandling and properly placed in the type-checking flow.
|
|
||
| // Export mapperFor for testing | ||
| module.exports = { mapperFor }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that tests handle file system side effects.
Exporting mapperFor enables testing, which is good. However, when this module is required (e.g., by tests), it will execute the babel transformation at lines 378-385, which writes to Definitions.js and docs.js. Ensure that your test setup either:
- Mocks the file system operations, or
- Runs in an isolated environment where these writes are acceptable, or
- Uses a test-specific entry point that only exports the function without running the build
🏁 Script executed:
#!/bin/bash
# Description: Check if tests mock fs operations or handle build side effects
# Look for test files that import buildConfigDefinitions
rg -n --type=js -A 5 "require.*buildConfigDefinitions|import.*buildConfigDefinitions"
# Check if tests mock fs.writeFileSync
rg -n --type=js -B 3 -A 3 "writeFileSync.*mock|mock.*writeFileSync|jest\.mock.*fs"Length of output: 471
🏁 Script executed:
#!/bin/bash
# Get full context of buildConfigDefinitions.js around the export and transformation
echo "=== buildConfigDefinitions.js lines 370-390 ==="
sed -n '370,390p' resources/buildConfigDefinitions.js
echo ""
echo "=== Check if transformation is wrapped in conditionals ==="
sed -n '375,385p' resources/buildConfigDefinitions.js
echo ""
echo "=== Check test file setup/teardown ==="
head -40 spec/buildConfigDefinitions.spec.jsLength of output: 2399
🏁 Script executed:
#!/bin/bash
# Check for NODE_ENV or test environment conditions in buildConfigDefinitions.js
rg -n "process\.env|NODE_ENV|process\.argv" resources/buildConfigDefinitions.js
# Check if there's a test helper or setup file
fd -e "js" -path "*/spec/*" -name "*setup*|*helper*|*config*" Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check if Jest is configured with setupFilesAfterEnv or similar that might mock fs
rg -n "jest|setupFiles|moduleNameMapper" jest.config.js package.json 2>/dev/null || echo "Config files not found in root, checking elsewhere"
# Look for jest configuration
fd -e "js" -o -e "json" -name "jest.config*" -o -name "package.json" -type f | head -5Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check if generated files are ignored or tracked
echo "=== Check .gitignore for src/Options/*.js ==="
grep -n "Options\|Definitions\|docs" .gitignore 2>/dev/null || echo "Pattern not found in .gitignore"
echo ""
echo "=== Check if generated files exist and their status ==="
ls -la src/Options/Definitions.js src/Options/docs.js 2>/dev/null || echo "Files not found (may not exist yet)"
echo ""
echo "=== Check if these files are versioned in git ==="
git ls-files src/Options/Definitions.js src/Options/docs.js 2>/dev/null || echo "Files not tracked"Length of output: 372
🏁 Script executed:
#!/bin/bash
# Check if there's jest config at root or package.json jest config
cat package.json | grep -A 20 '"jest"' 2>/dev/null || echo "No jest config in package.json"
# Also try to find jest config files
find . -maxdepth 2 -name "jest.config.*" 2>/dev/nullLength of output: 30
🏁 Script executed:
#!/bin/bash
# Check if buildConfigDefinitions.js is only executed during build, not by tests
# Look for where buildConfigDefinitions.js is actually imported/used
rg -n "buildConfigDefinitions" --type=js -B 2 -A 2 | head -50Length of output: 1499
Refactor to prevent file system writes during test import.
The side effect concern is confirmed: the transformation at lines 378-385 executes unconditionally on import, writing to versioned files src/Options/Definitions.js and src/Options/docs.js every time tests run.
Use one of these approaches:
- Add
if (require.main === module)guard around the transformation, so it only runs when executed directly - Add
NODE_ENV === 'test'check to skip transformation during testing - Mock
fs.writeFileSyncin test setup - Create a separate test export that only exports
mapperForwithout running the build
🤖 Prompt for AI Agents
In resources/buildConfigDefinitions.js around lines 378–388, the unconditional
transformation/writes run on module import (causing file system writes during
tests); guard the transformation so it only runs when executed directly (e.g.,
wrap the write/transform block in if (require.main === module) or skip when
process.env.NODE_ENV === 'test'), leaving module.exports = { mapperFor } intact
so tests can import mapperFor without triggering any file writes.
3 participants
Pull Request
Issue
https://github.com/parse-community/parse-server/issues/9907
The issue occurs because the
mapperForfunction inbuildConfigDefinitions.jsdid not include a specific case for theObjectTypeAnnotationtype in Flow definitions.As a result, configuration options defined as Flow objects - such as:
auth: ?{ [string]: AuthAdapter };were not mapped to the appropriate parser (objectParser) when generating CLI definitions.
This caused environment variables like PARSE_SERVER_AUTH_PROVIDERS to be treated as raw strings instead of being parsed as JSON objects.
In practice, this broke the built-in Microsoft authentication adapter (and any other adapter relying on object-type auth configs), throwing errors such as:
Error: Microsoft options are required.The missing objectParser action in the generated definitions.js file prevented the environment variable from being correctly parsed into an object structure.
Approach
mapperForfunction inbuildConfigDefinitions.jsto handleObjectTypeAnnotationby mapping it to theobjectParser, ensuring object types are parsed correctly.objectParseraction to theauthoption inParseServerOptionsinDefinitions.js, enabling proper parsing of theauthconfiguration as an object.Tasks
Summary by CodeRabbit
Bug Fixes
Tests